Fix race in tidy status with cert counting #18899
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When generating the tidy status, we read the values of two backend
atomics, b.certCount and b.revokedCertCount, without using the atomic
load operation. This resulted in a data race when the status was read
at the same time as an on-going tidy operation:
WARNING: DATA RACE
Write at 0x00c00c77680c by goroutine 90522:
sync/atomic.AddInt32()
/usr/local/go/src/runtime/race_amd64.s:281 +0xb
sync/atomic.AddUint32()
<autogenerated>:1 +0x1a
github.com/hashicorp/vault/builtin/logical/pki.(*backend).tidyStatusIncRevokedCertCount()
/home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:1236 +0x107
github.com/hashicorp/vault/builtin/logical/pki.(*backend).doTidyRevocationStore()
/home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:525 +0x1404
github.com/hashicorp/vault/builtin/logical/pki.(*backend).startTidyOperation.func1.1()
/home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:290 +0x1a4
github.com/hashicorp/vault/builtin/logical/pki.(*backend).startTidyOperation.func1()
/home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:342 +0x278
Previous read at 0x00c00c77680c by goroutine 90528:
reflect.Value.Uint()
/usr/local/go/src/reflect/value.go:2584 +0x195
encoding/json.uintEncoder()
/usr/local/go/src/encoding/json/encode.go:562 +0x45
encoding/json.ptrEncoder.encode()
/usr/local/go/src/encoding/json/encode.go:944 +0x3c2
encoding/json.ptrEncoder.encode-fm()
<autogenerated>:1 +0x90
encoding/json.(*encodeState).reflectValue()
/usr/local/go/src/encoding/json/encode.go:359 +0x88
encoding/json.interfaceEncoder()
/usr/local/go/src/encoding/json/encode.go:715 +0x17b
encoding/json.mapEncoder.encode()
/usr/local/go/src/encoding/json/encode.go:813 +0x854
... more stack trace pointing into JSON encoding and http
handler...
In particular, because the tidy status was directly reading the uint
value without resorting to the atomic side, the JSON serialization could
race with a later atomic update.
Signed-off-by: Alexander Scheel <[email protected]>
Because no tidy operation is running here, it should be safe to read the pointed value directly, but use the safer atomic.Load for consistency. Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
sgmiller
approved these changes
Jan 30, 2023
stevendpclark
approved these changes
Jan 30, 2023
|
|
|
Thanks all! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Read total cert counts with atomic.LoadUint32(...)
When generating the tidy status, we read the values of two backend
atomics, b.certCount and b.revokedCertCount, without using the atomic
load operation. This resulted in a data race when the status was read
at the same time as an on-going tidy operation:
In particular, because the tidy status was directly reading the uint
value without resorting to the atomic side, the JSON serialization could
race with a later atomic update.
Also addresses a similar case in the tests, but this shouldn't race as its at the end of the test and no tidy should still be running.